feat: allow copy config from existing configs#6785
feat: allow copy config from existing configs#6785Flartiny wants to merge 2 commits intoAstrBotDevs:masterfrom
Conversation
dosubot
bot
added
the
size:L
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 此拉取请求为配置管理页面带来了显著改进，引入了便捷的配置复制功能，使用户能够基于现有配置快速创建新配置，从而大幅减少了手动重复工作。同时，为了解决配置名称可能重复的问题，前端现在强制执行名称唯一性校验，并在用户尝试使用已存在的名称时提供即时反馈。这些更新共同提升了用户体验，使配置管理流程更加高效、直观且不易出错。 Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
dosubot
bot
added
the
area:webui
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- There is quite a bit of duplicated logic for initializing/resetting the config form state across
startCreateConfig,startEditConfig,startCopyConfig, andcancelConfigForm; consider extracting a single helper (e.g.resetConfigForm({ mode, config })) to avoid future drift and make the form behavior easier to reason about. - Name normalization is currently performed in several places (
saveConfigForm,createNewConfig,updateConfigInfo,copyConfig); sincesaveConfigFormalready normalizes and writes back toconfigFormData.name, you could pass the normalized name down as an argument to the create/update/copy helpers and avoid repeated normalization and potential inconsistencies.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There is quite a bit of duplicated logic for initializing/resetting the config form state across `startCreateConfig`, `startEditConfig`, `startCopyConfig`, and `cancelConfigForm`; consider extracting a single helper (e.g. `resetConfigForm({ mode, config })`) to avoid future drift and make the form behavior easier to reason about.
- Name normalization is currently performed in several places (`saveConfigForm`, `createNewConfig`, `updateConfigInfo`, `copyConfig`); since `saveConfigForm` already normalizes and writes back to `configFormData.name`, you could pass the normalized name down as an argument to the create/update/copy helpers and avoid repeated normalization and potential inconsistencies.
## Individual Comments
### Comment 1
<location path="dashboard/src/views/ConfigPage.vue" line_range="160" />
<code_context>
<v-btn variant="text" @click="cancelConfigForm">{{ tm('buttons.cancel') }}</v-btn>
<v-btn color="primary" @click="saveConfigForm"
- :disabled="!configFormData.name">
+ :disabled="!configFormData.name || (isCopyingConfig && !copySourceConfigId)">
{{ isEditingConfig ? tm('buttons.update') : tm('buttons.create') }}
</v-btn>
</code_context>
<issue_to_address>
**issue (bug_risk):** The save button can be enabled for whitespace-only names, leading to a validation error on submit.
`saveConfigForm` trims the name via `normalizeConfigName` and rejects empty results, but the disabled state uses the raw `configFormData.name`. With a whitespace-only name the button enables, then submit fails. Base `:disabled` on the trimmed value instead (e.g. `!normalizeConfigName(configFormData.name)`, maybe via a computed property) so the button state matches the validation logic.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a useful feature for copying existing configurations, which will save users time. It also adds important frontend validation to prevent duplicate configuration names. The implementation is solid. I've provided a couple of suggestions to refactor parts of the Vue component for better readability and maintainability, such as using a computed property for a complex template expression and converting a promise chain to async/await.
dashboard/src/views/ConfigPage.vue
Outdated
| <h3 class="mb-4"> | ||
| {{ isEditingConfig ? tm('configManagement.editConfig') : (isCopyingConfig ? tm('configManagement.copyConfig') : tm('configManagement.newConfig')) }} | ||
| </h3> |
There was a problem hiding this comment.
The logic for determining the form title is getting a bit complex to be inlined in the template. To improve readability and maintainability, consider extracting this logic into a computed property.
For example, you could add a configFormTitle computed property:
computed: {
// ... other computed properties
configFormTitle() {
if (this.isEditingConfig) {
return this.tm('configManagement.editConfig');
}
if (this.isCopyingConfig) {
return this.tm('configManagement.copyConfig');
}
return this.tm('configManagement.newConfig');
}
}Then, you can simplify the template:
<h3 class="mb-4">{{ configFormTitle }}</h3>
dashboard/src/views/ConfigPage.vue
Outdated
| copyConfig() { | ||
| const configName = this.normalizeConfigName(this.configFormData.name); | ||
| axios.get('/api/config/abconf', { | ||
| params: { id: this.copySourceConfigId } | ||
| }).then((res) => { | ||
| const sourceConfig = res.data?.data?.config; | ||
| if (!sourceConfig) { | ||
| this.save_message = this.tm('configManagement.copyFailed'); | ||
| this.save_message_snack = true; | ||
| this.save_message_success = "error"; | ||
| return; | ||
| } | ||
| return axios.post('/api/config/abconf/new', { | ||
| name: configName, | ||
| config: sourceConfig | ||
| }); | ||
| }).then((res) => { | ||
| if (!res) return; | ||
| if (res.data.status === "ok") { | ||
| this.save_message = res.data.message; | ||
| this.save_message_snack = true; | ||
| this.save_message_success = "success"; | ||
| this.getConfigInfoList(res.data.data.conf_id); | ||
| this.cancelConfigForm(); | ||
| } else { | ||
| this.save_message = res.data.message; | ||
| this.save_message_snack = true; | ||
| this.save_message_success = "error"; | ||
| } | ||
| }).catch((err) => { | ||
| console.error(err); | ||
| this.save_message = err?.response?.data?.message || this.tm('configManagement.copyFailed'); | ||
| this.save_message_snack = true; | ||
| this.save_message_success = "error"; | ||
| }); | ||
| }, |
There was a problem hiding this comment.
The copyConfig method uses promise chaining with .then(). While this works, refactoring it to use async/await would make the code more linear, readable, and easier to debug. The error handling can also be simplified with a single try...catch block.
async copyConfig() {
const configName = this.normalizeConfigName(this.configFormData.name);
try {
const getRes = await axios.get('/api/config/abconf', {
params: { id: this.copySourceConfigId }
});
const sourceConfig = getRes.data?.data?.config;
if (!sourceConfig) {
throw new Error('Failed to fetch source configuration for copying.');
}
const postRes = await axios.post('/api/config/abconf/new', {
name: configName,
config: sourceConfig
});
if (postRes.data.status === "ok") {
this.save_message = postRes.data.message;
this.save_message_snack = true;
this.save_message_success = "success";
this.getConfigInfoList(postRes.data.data.conf_id);
this.cancelConfigForm();
} else {
throw new Error(postRes.data.message || this.tm('configManagement.copyFailed'));
}
} catch (err) {
console.error(err);
this.save_message = err?.response?.data?.message || err.message || this.tm('configManagement.copyFailed');
this.save_message_snack = true;
this.save_message_success = "error";
}
},
- duplicated logic for initializing/resetting the config - check whitespace-only names
|
@sourcery-ai review |
1 participant
feat mentioned by #4020
允许从已存在的配置文件中复制，减少重复操作。
并且注意到此前是允许配置文件重名的，不利于辨识，故前端添加了校验（应该不影响已经创建的重名文件）。
Modifications / 改动点
dashboard/src/views/ConfigPage.vue
以及i18n（RU没能力做进一步校验了）
Screenshots or Test Results / 运行截图或测试结果
Checklist / 检查清单
😊 If there are new features added in the PR, I have discussed it with the authors through issues/emails, etc.
/ 如果 PR 中有新加入的功能，已经通过 Issue / 邮件等方式和作者讨论过。
👀 My changes have been well-tested, and "Verification Steps" and "Screenshots" have been provided above.
/ 我的更改经过了良好的测试，并已在上方提供了“验证步骤”和“运行截图”。
🤓 I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations in
requirements.txtandpyproject.toml./ 我确保没有引入新依赖库，或者引入了新依赖库的同时将其添加到
requirements.txt和pyproject.toml文件相应位置。😮 My changes do not introduce malicious code.
/ 我的更改没有引入恶意代码。
Summary by Sourcery
Add ability to create new configs by copying existing ones and improve config name handling in the dashboard.
New Features:
Enhancements: